feat(api): Track 3.2 — X-Admin-Key gates cross-user DELETE on /api/analysis-runs - #37
Conversation
…alysis-runs
Second of four pieces from Track 3. Adds an operator-level privilege
gate to DELETE /api/analysis-runs/{run_id}.
Semantics:
- Admin key BYPASSES ownership (does not replace it). Users can still
delete their own runs without the header — current behavior preserved.
- New SONIC_ANALYZER_ADMIN_KEY env var. If unset (default, local mode),
no admin path exists and ownership remains the only gate.
- If set, requests carrying matching X-Admin-Key header can delete any
run regardless of owner. Comparison uses hmac.compare_digest for
constant-time resistance to timing-based key probing.
- Both env value and header value are whitespace-stripped before
compare so secret-manager trailing newlines don't break matching.
- Response now includes 'deletedBy': 'owner' | 'admin' so audit logs
can distinguish the two paths.
Files:
- apps/backend/auth_context.py — two new pure functions:
admin_key_is_configured() (env-var presence check, for routing
decisions) and admin_key_matches(provided) (the auth decision).
ADMIN_KEY_ENV_VAR constant published at module scope.
- apps/backend/server.py — delete_analysis_run accepts X-Admin-Key
header. When it matches, runtime.get_run(owner_user_id=None) skips
the ownership check. 404 RUN_NOT_FOUND is still returned for non-
existent runs (no run-id enumeration via the admin path).
- apps/backend/tests/test_auth_context.py (new, 11 tests, all pass
locally) — covers admin_key_is_configured (unset, empty, whitespace,
set) and admin_key_matches (env unset, none provided, exact match,
case mismatch, partial match, whitespace stripping, hmac.compare_digest
is actually wired in).
- apps/backend/tests/test_server.py (+138 lines, 3 tests) — covers
admin-key bypasses ownership for non-owner caller; wrong key falls
back to ownership (returns RUN_NOT_FOUND for non-owner); env var
unset closes the admin path even with a header.
- CLAUDE.md / ARCHITECTURE.md — documented as backend-only env var.
Out of scope (deferred):
- X-Admin-Key on a forthcoming bulk-list endpoint
- X-Admin-Key on /interpretations / spectral-enhancements (admin
doesn't have a clear use case there yet)
https://claude.ai/code/session_01Uh2eNT1oqjixcox8UX3BTg
slittycode
left a comment
slittycode
left a comment
There was a problem hiding this comment.
Verdict: APPROVE
Summary
Adds an operator-level X-Admin-Key gate to DELETE /api/analysis-runs/{run_id}. The implementation is correctly partitioned: two pure functions in auth_context.py handle all auth logic; server.py is just routing. All 11 test_auth_context tests pass locally. Three route-level tests cover the admin-bypass, wrong-key-fallback, and unset-env-var paths; they require FastAPI's venv to run, which isn't present in this environment, but the test logic is correct from the diff.
Findings
Worth considering:
admin_key_is_configured() is defined and documented but not called in server.py — only admin_key_matches() is used. Fine; the docstring frames it as a routing-decision helper for future conditional route exposure. Noting it so nobody removes it thinking it's dead code.
The test_delete_analysis_run_admin_key_unset_ignores_header test calls server.os.environ.pop("SONIC_ANALYZER_ADMIN_KEY", None) inside the patch.dict block to handle environments where the var might already be set. That's the correct approach when patch.dict(clear=False) is in use — no issue.
Test results
11 passed / 0 failed (tests.test_auth_context). Server route tests not runnable without FastAPI venv in this environment; auth logic tests are exhaustive.
Phase boundary check
Clean. No Phase 1 measurements touched. No Phase 2/3 files modified. Purely infra.
Generated by Claude Code
slittycode
left a comment
slittycode
left a comment
There was a problem hiding this comment.
Verdict: APPROVE
Summary
Adds an optional operator-level privilege gate (SONIC_ANALYZER_ADMIN_KEY) to DELETE /api/analysis-runs/{run_id}. Default behavior is fully unchanged — when the env var is unset the admin path is closed and ownership is the only gate. The security-critical comparison uses hmac.compare_digest and the test suite includes a regression guard that it stays wired in. Test coverage is solid: 11 pure-logic tests in the new test_auth_context.py plus 3 new route-level integration tests. Both Backend and Frontend CI checks pass.
Findings
Worth considering (non-blocking):
server.py now has both import auth_context (new, line ~29) and from auth_context import AuthenticationRequiredError, UserContext, resolve_api_user_context (existing). The module-level import is used so the call site reads auth_context.admin_key_matches(...) rather than a bare function — reasonable intentional choice, not a bug.
The admin-bypass test exercises the "different user + matching key" case. The PR description's table also lists "no user header at all" as a valid admin scenario; that sub-case isn't separately tested. The code path is identical (both skip _resolve_route_user_context), so this isn't a correctness gap — mentioning in case you want the coverage to match the table.
Test results
Backend: 11 new auth_context tests + 3 new server route tests + 1 assertion added to existing delete test. All pass. CI: Backend ✅ Frontend ✅.
Phase boundary check
Clean. No changes to Phase 1 DSP code, Phase 1 output schema, or Phase 2/3 interpretation logic.
Generated by Claude Code
Adds the coverage case flagged in PR #37 review: the PR description table claims the admin key bypasses user-context resolution entirely. This pins that claim with x_asa_user_id=None — a request that would fail under hosted-mode ownership rules still succeeds when the admin key matches. Not a correctness fix; the code path was already identical to the existing 'different user + matching key' test. Closing the doc/test asymmetry so the PR table is verifiable. https://claude.ai/code/session_01Uh2eNT1oqjixcox8UX3BTg
Fourth and final piece of Track 3 from the merged external-repo review.
Additive over the existing 8-state stage status — does not change or
remove anything.
What's new:
- apps/backend/stage_status.py (new) — pure module with the canonical
8 -> 5 mapping table, the to_public_status() helper, and
PUBLIC_STATUS_VALUES exported for type generation / docs.
Mapping (internal -> public):
queued -> queued
running -> running
blocked -> queued (transient scheduling state)
ready -> queued (scheduled but not yet running)
completed -> completed
failed -> failed
interrupted -> interrupted
not_requested -> null (stage not in pipeline)
- apps/backend/server_phase1.py — _normalize_run_snapshot now also
annotates each stage with publicStatus via the new
_annotate_public_status helper. The original status field is
preserved untouched.
- apps/backend/tests/test_stage_status.py (new, 16 tests) — pins
every cell of the mapping, the public-value set, defensive cases
(None, unknown, empty, case-sensitive), and a completeness check
that fails if the set of internal states drifts without an explicit
mapping update.
- apps/backend/tests/test_server.py (+1 test) — integration test
that GET /api/analysis-runs/{run_id} returns publicStatus on all
three stages with the correct values for a fresh run.
- apps/ui/src/types/backend.ts — new PublicStageStatus type (the
5-value union). publicStatus: PublicStageStatus | null added to
MeasurementStageSnapshot, PitchNoteTranslationStageSnapshot,
InterpretationStageSnapshot. attempt summaries unchanged
(publicStatus is only on top-level stages).
- apps/ui/src/services/analysisRunsClient.ts — new
parsePublicStageStatus helper + PUBLIC_STAGE_STATUSES set; threaded
into the three stage parsers. The helper accepts null and missing
values without throwing so legacy snapshots that pre-date the
field continue to deserialize.
- docs/adr/0001-phase1-json-schema-v1.md — documents the additive
field, the mapping table, and the rationale (internal scheduling
states are scheduling concerns the client doesn't need to act on).
Out of scope (deferred):
- Removing publicStatus duplication from the internal status field
later. The additive approach was the explicit choice; collapsing
to one field would be a breaking change requiring a v2 schema bump.
- Frontend UI that reads publicStatus. Existing UI continues to use
the 8-state status field; publicStatus is the additive surface for
future external consumers and any new UI views that prefer the
smaller vocabulary.
This closes Track 3 (all four pieces shipped):
- 3.1 URL ingestion (#36, merged)
- 3.2 X-Admin-Key on DELETE (#37, merged)
- 3.3 GET /source-audio re-serve (#40, merged)
- 3.4 publicStatus additive (this PR)
https://claude.ai/code/session_01Uh2eNT1oqjixcox8UX3BTg
Co-authored-by: Claude <noreply@anthropic.com>
2 participants
Second of four pieces from Track 3. Adds an operator-level privilege gate to
DELETE /api/analysis-runs/{run_id}so operators can purge runs that don't belong to them.Semantics
The admin key bypasses ownership rather than replacing it. Users can still delete their own runs without the header — existing behavior is preserved. Operators with the configured key can delete any run.
SONIC_ANALYZER_ADMIN_KEYset?X-Admin-Keyheader matches?RUN_NOT_FOUND.RUN_NOT_FOUNDis still returned for nonexistent runs regardless of the admin key — the admin path cannot be used to enumerate run IDs (response is identical whether the run doesn't exist or the caller is denied).Response now includes
"deletedBy": "owner" | "admin"so audit logs can distinguish the two paths.Security details
hmac.compare_digestfor constant-time comparison — resists timing-based probing of the configured key. There's a test that verifies this is actually wired in (so nobody "simplifies" it back to==).Files
apps/backend/auth_context.pyadmin_key_is_configured()(presence check for routing decisions) andadmin_key_matches(provided)(the auth decision).ADMIN_KEY_ENV_VARconstant at module scope.apps/backend/server.pydelete_analysis_runacceptsX-Admin-Keyheader. When it matches,runtime.get_run(owner_user_id=None)skips the ownership check.apps/backend/tests/test_auth_context.py(new, 11 tests)hmac.compare_digestis actually called.apps/backend/tests/test_server.py(+3 tests, 138 lines)RUN_NOT_FOUND); env var unset closes the admin path even with a header. Existingtest_delete_analysis_run_removes_owned_runtest extended to assertdeletedBy: "owner".CLAUDE.md/ARCHITECTURE.mdVerification
11 auth_context tests pass locally (Python stdlib only):
3 route tests run via the existing
AnalysisRuntime+patch.object(server, "get_analysis_runtime", ...)pattern. CI will exercise them.What this PR does NOT do
DELETEand a "forthcoming admin list." This PR scopes toDELETEonly. AddingX-Admin-Keyto a bulk-list / stats endpoint is a separate follow-up if/when those endpoints exist.deletedByfield is in the response but isn't written to an audit table. The existinganalysis_runsrow is deleted; an admin audit trail is a hosted-mode feature for a later PR.Track 3 sequencing
X-Admin-KeyonDELETEGET /api/analysis-runs/{run_id}/source-audiore-servehttps://claude.ai/code/session_01Uh2eNT1oqjixcox8UX3BTg
Generated by Claude Code